Create new `OP_MULTIPARAM` to implement subroutine signatures #23574

leonerd · 2025-08-14T19:43:27Z

Creates a new UNOP_AUX op type, OP_MULTIPARAM, that handles all of the initial behaviour of assigning values to parameters of a subroutine signature out of values passed by the caller. This is created in a similar style to other multi-ops like OP_MULTIDEREF and OP_MULTICONCAT where the op's aux structure contains a sub-program of sorts, which describes all of the small details of operation.

Also adds a LOGOP, OP_PARAMTEST and UNOP OP_PARAMSTORE which are responsible for implementing the default expressions of optional parameters. These use the SvPADSTALE flag set on pad lexicals used as parameters to remember whether assignment has happened, ensuring that missing vs present-but-undef can be detected in a way that does not depend on counting arguments on the stack.

This change is carefully designed to support two future ideas:

Named parameters as per PPC0024
"no-snails"; the performance optimisation that avoids storing incoming argument values in the @_ AV and instead consumes them directly from the stack

--

This set of changes currently does not have a perldelta entry, because it doesn't directly make any changes that are end-user visible. However, since it is quite a significant internal change, I could be talked into writing an entry in the "internal changes" section anyway. Reviewers: Thoughts?

leonerd · 2025-08-14T21:14:48Z

Weirdly, while github CI and most commentors on IRC report test failures in t/op/signatures.t, this runs reliably on my local machine. Very strange.

In case it's somehow related to build options, the script I use to configure and build is:

test -f config.sh && rm config.sh
test -f Policy.sh && rm Policy.sh

./Configure -des \
  -Dusedevel \
  -Dprefix=$HOME/perl5/perlbrew/perls/bleadperl/ \
  -DDEBUGGING \
  -Uversiononly \
  -Dman1dir=none -Dman3dir=none \
  -Dusethreads \
  -Dinc_version_list=none \
  -Doptimize='-gdwarf-2 -g3' \
  "$@"

make -j8
make -j8 test_prep

mauke

Commit message: "This two" should be "These two"

mauke · 2025-08-15T02:14:46Z

op.c

             */
-            varop->op_next   = defop;
-            defexpr->op_next = varop;
+            param->padix = allocmy("", 0, 0);


Perl_allocmy currently does not support empty names because it blindly uses name[1]. It needs to be changed to check len first.

allocmy() is just a wrapper around pad_add_name_pvn() with some extra sanity checking and flag-setting (I think it exists just for a convenience from the parser). I've instead changed this code to call pad_add_name_pvn() directly.

Adding more asserts in #23583 which would have caught this

Recent discussons on github [1] found a bug when calling this function as allocmy("", 0, 0) This ought not be allowed. The length must be at least 2, because the function checks the first two characters of `name`. [1]: Perl#23574 (comment)

Recent discussons on github [1] found a bug when calling this function as allocmy("", 0, 0) This ought not be allowed. The length must be at least 2, because the function checks the first two characters of `name`. [1]: #23574 (comment)

The previous names were just copied from similar fields in previous signature handling code. These new names are a better fit for their current and intended future purpose.

By defining a helper term for optional defaulting expression we can remove one of the three alternate cases down to just two. This will help when adding more code to these action blocks in future,

These two new helper functions will be useful for sharing behaviour with upcoming code, when adding OP_MULTIPARAM.

…gument processing The specific behaviour of this obscure modification case is not documented or relied upon elsewhere in code. Rather than attempt to preserve this cornercase, it's easier just to no longer test for it, as upcoming changes will alter the values that are visible.

Creates a new UNOP_AUX op type, `OP_MULTIPARAM`, that handles all of the initial behaviour of assigning values to parameters of a subroutine signature out of values passed by the caller. This is created in a similar style to other multi-ops like `OP_MULTIDEREF` and `OP_MULTICONCAT` where the op's aux structure contains a sub-program of sorts, which describes all of the small details of operation. Also adds a LOGOP, `OP_PARAMTEST` and UNOP `OP_PARAMSTORE` which are responsible for implementing the default expressions of optional parameters. These use the `SvPADSTALE` flag set on pad lexicals used as parameters to remember whether assignment has happened, ensuring that missing vs present-but-undef can be detected in a way that does not depend on counting arguments on the stack. This change is carefully designed to support two future ideas: * Named parameters as per PPC0024 https://github.com/Perl/PPCs/blob/main/ppcs/ppc0024-signature-named-parameters.md * "no-snails"; the performance optimisation that avoids storing incoming argument values in the `@_` AV and instead consumes them directly from the stack

bulk88 · 2025-08-15T15:00:37Z

ext/B/B.xs

+                EXTEND(SP, (IV)(3 + nparams + 1));
+                mPUSHu(p->min_args);
+                mPUSHu(p->n_positional);
+                PUSHs(sv_2mortal(p->slurpy ? newSVpvf("%c", p->slurpy) : &PL_sv_no));


Why run a 1 byte long string through a printf engine?

Because offhand I don't believe we have a newSVpvc function. Can you suggest an alternative?

bulk88 · 2025-08-15T15:01:59Z

ext/B/B.xs

+            {
+                struct op_multiparam_aux *p = (struct op_multiparam_aux *)aux;
+                UV nparams = p->n_positional;
+                EXTEND(SP, (IV)(3 + nparams + 1));


Add the MEXTEND mortal stack stretcher macro here too.

bulk88 · 2025-08-15T15:02:41Z

ext/B/B.xs

+                for(UV parami = 0; parami < nparams; parami++)
+                    mPUSHu(p->param_padix[parami]);
+                mPUSHu(p->slurpy_padix);
+                XSRETURN(3 + nparams + 1);


replace with PUTBACK; return;

bulk88 · 2025-08-15T15:13:35Z

ext/XS-APItest/APItest.xs

+                    if(namepv)
+                        sv_catpvf(retsv, ":%s=%" UVf, namepv, paramidx);
+                    else
+                        sv_catpvf(retsv, ":(anon)=%" UVf, paramidx);


#define PadnamePV(pn) (pn)->xpadn_pv #define PadnameLEN(pn) (pn)->xpadn_len #define PadnameUTF8(pn) 1 #define PadnameSV(pn) newSVpvn_flags(PadnamePV(pn), PadnameLEN(pn), SVs_TEMP|SVf_UTF8)

":(anon)=%" can be de-duped to 1 branch above with ":(%s)=%". "anon",

in theory PadnameLEN(pn) and sv_catXXX() family calls should be taken advantage of here, rather than repeatedly going through the printf engine.

IDK and IDC enough, and probably its impossible to write a bug ticket with a failure/defect demo, about it, but I see UTF8 flag is perma-on in the data source API, but we arent propagating it to the higher level. And this is XS::APItest anyways, so perfection isnt critical. but someone might look at this in the future for "best practices" ideas, and then copy paste quicky hacky code into a more visible API.

bulk88 · 2025-08-15T15:16:02Z

lib/B/Deparse.pm

+        # Look for OP_NULL[OP_PARAMTEST[OP_PARAMSTORE]]
+        if ($o->name eq 'null' and $o->flags & OPf_KIDS and
+                $o->first->name eq 'paramtest' and
+                $o->first->first->name eq 'paramstore') {


$o->first cache the retval of the method or key ($first = $o->first)->name.

bulk88 · 2025-08-15T15:19:01Z

lib/B/Deparse.pm

+
+            length $sig[$parami] > 1 ?
+                ( $sig[$parami] .= ' ' ) :
+                ( $sig[$parami] = '$' ); # intentionally no trailing space


dont look up same thing over and over in an array, do a $sref = \$sig[$parami]; or $s = $sig[$parami];. probably the first is easier in this sub.

bulk88 · 2025-08-15T15:19:45Z

lib/B/Deparse.pm

+
+            my $defop = $paramtest->first->first;
+            if ($defop->name eq "stub") {
+                $sig[$parami] .= "$assign";


bulk88 · 2025-08-15T15:23:50Z

op.c

    char        slurpy;     /* the sigil of the slurpy var (or null) */
-    OP         *elemops;    /* NULL, or an OP_LINESEQ of individual element and fence ops */


move field char slurpy; to the bottom of the struct. don't have hidden alignment holes.

bulk88 · 2025-08-15T15:26:03Z

op.c

+     * zero or more arguments.
+     */
+    UV          next_argix; /* the argument index of the next parameter we add */
+    UV          opt_params; /* number of optional scalar parameters */


Why are we using 64 bit integers to store an arrays length on i386 CPUs? Since when is void * 8 bytes long on a Pentium 3?

bulk88 · 2025-08-15T15:28:15Z

op.c

-    if(signature->elemops)
-        op_free(signature->elemops);
+    if(signature->params) {
+        for(UV parami = 0; parami < signature->nparams; parami++) {


i386 doesnt have 8 byte pointers, use Size_t.

bulk88 · 2025-08-15T15:31:38Z

op.c

            /* handle '$=' special case */
-            if(varop)
+            if(padix)
                yyerror("Optional parameter lacks default expression");


was struct yy_parser_signature_param *param = subsignature_push_param(); leaked if this error branch was entered?

bulk88 · 2025-08-15T15:34:03Z

op.c

+    UV end_argix = signature->next_argix;
+
+    struct op_multiparam_aux *aux = (struct op_multiparam_aux *)PerlMemShared_malloc(
+            sizeof(struct op_multiparam_aux) + end_argix * sizeof(PADOFFSET));


I hope I know PEMDAS but a () would be nice and make it easier to read, on what gets multiplied.

bulk88 · 2025-08-15T15:39:10Z

op.c

+        /* Move the entire chain of kid ops in one go */
+        OpMORESIB_set(cLISTOPx(sigops)->op_last, fenceops->op_first);
+        cLISTOPx(sigops)->op_last = fenceops->op_last;
+        OpLASTSIB_set(cLISTOPx(sigops)->op_last, sigops);


factor out cLISTOPx(sigops) to a C auto i suspect its many derefs deep and keeps being re-read after each of these assignments in -O1/-O2. cLISTOPx(sigops)->op_last is written so many times its a puzzle what happened to the old target ptr struct, and the new target ptr, and is new or old target inside the container after this block.

bulk88 · 2025-08-15T15:40:13Z

op.c

+    struct yy_parser_signature_param *params = signature->params;
+    UV max_argix = 0;
+
+    for(UV parami = 0; parami < signature->nparams; parami++) {


no 64 bit pointers on ARM32/i386.

bulk88 · 2025-08-15T15:43:00Z

op.c

+        PADOFFSET padix = param->padix;
+
+        while(max_argix < argix) {
+            aux->param_padix[max_argix] = 0;


don't double deref in a loop. CC has no idea if &(aux->param_padix) and &(aux->param_padix[5]) happened to be the same mem addr.

IDK what realistic values are for max_argix, but if its over 16 x U32/U64 or 32 x U32/U64 call libc's memset aka perl Zero(), below that, this loop will win in speed b/c its move 4/8 aligned bytes at a time, while all libc memsets, need 4-8 branches to figure out alignment and if to use 2/4/8/16 sse/32 avx/64 avx512 loops.

bulk88 · 2025-08-15T15:50:59Z

op.c

+            OP *o = newUNOP(OP_NULL, 0, paramtest);
+
+            paramstore->op_next = o;
+            paramtest->op_next = o;


are these zeroings really needed? use a step debugger to find out whats inside. are they already zeroed b/c of their earlier allocator() always zering new structs? is it uninit memory? was there a resource tracked ptr here that we need to free in this slot before zering?

bulk88 · 2025-08-15T15:52:46Z

op.h

+    UV    n_positional; /* = the number of mandatory + optional scalar parameters, not counting a final slurpy */
+    char  slurpy;
+    PADOFFSET *param_padix; /* points at storage allocated along with the struct itself, immediately following */
+    PADOFFSET slurpy_padix;


sort all struct fields by alignment and size, dont use 64 bit ptrs on 32 bit CPUs

bulk88 · 2025-08-15T15:54:54Z

pp.c

+STATIC void
+S_av_refresh_elements_range(pTHX_ AV *av, IV startix, IV endix)
+{
+    for(IV ix = startix; ix < endix; ix++) {


AV*s use SSize_t not IV.

bulk88 · 2025-08-15T15:56:49Z

pp.c

+S_av_refresh_elements_range(pTHX_ AV *av, IV startix, IV endix)
+{
+    for(IV ix = startix; ix < endix; ix++) {
+        SV **svp = av_fetch(av, ix, FALSE);


Unless var av is reachable from PP lang, use fetch_simple() variants, we dont need to call TIEARRAY PP methods here.

bulk88 · 2025-08-15T15:59:54Z

pp.c

+{
+    for(IV ix = startix; ix < endix; ix++) {
+        SV **svp = av_fetch(av, ix, FALSE);
+        SV *newsv = newSVsv_flags(svp ? *svp : &PL_sv_undef,


It takes alot of work for sv_setsv_flags() to figure out &PL_sv_undef is empty. This isn't Chrome or NodeJS, its C. It won't constant fold. Grep the perl repo for the correct way to make a new read write undef SV*.

bulk88 · 2025-08-15T16:13:18Z

pp.c

+    for(IV ix = startix; ix < endix; ix++) {
+        SV **svp = av_fetch(av, ix, FALSE);
+        SV *newsv = newSVsv_flags(svp ? *svp : &PL_sv_undef,
+                (SV_DO_COW_SVSETSV|SV_NOSTEAL));


SV_DO_COW_SVSETSV good idea

bulk88 · 2025-08-15T16:17:55Z

pp.c

+        SV **svp = av_fetch(av, ix, FALSE);
+        SV *newsv = newSVsv_flags(svp ? *svp : &PL_sv_undef,
+                (SV_DO_COW_SVSETSV|SV_NOSTEAL));
+        if(!av_store(av, ix, newsv))


Is av a TIEARRAY? Is this really going to return false? Why are we continuing execution and not throwing a panic:/heap corruption/OOM? Can it be a TIEARRAY? why not use av_simpleX() family of calls here?

Renew() Newx() handles all OOMs NULL RETVALS for us unless we explicitly temporarily disabled that, which is done approximately in only 1 place inside the entire P5P repo, and ~1-2 places on grep CPAN.

bulk88 · 2025-08-15T16:20:44Z

pp.c

+#define av_refresh_elements_range(av, startix, endix)  S_av_refresh_elements_range(aTHX_ av, startix, endix)
+STATIC void
+S_av_refresh_elements_range(pTHX_ AV *av, IV startix, IV endix)
+{


Where is the av_extend()? so we aren't constantly going inside libc's realloc every 1-5 SVs. I forgot the exact increment unit av_store() does on a grow event, but even wanting know that count is UB/bad coding practices. av_extend() it once. We know the max length, we aren't iterating a HV or a SQL DB or a readdir() or something off an AJAX socket here.

bulk88 · 2025-08-15T16:23:37Z

pp.c

+
+    UV parami;
+    for(parami = 0; parami < nparams; parami++) {
+        PADOFFSET padix = aux->param_padix[parami];


factor out of the loop reading's struct aux's param_padix field over and over , nothing will be reallocing it afaik

bulk88 · 2025-08-15T16:26:41Z

pp.c

+        }
+
+        SV **valp = av_fetch(defav, parami, FALSE);
+        SV *val = valp ? *valp : &PL_sv_undef;


When is valp retval going to be NULL?
SV* val should be set to NULL instead of &PL_sv_undef here if SV** was NULL.

bulk88 · 2025-08-15T16:31:57Z

pp.c

+        if (UNLIKELY(TAINT_get) && !SvTAINTED(val))
+            TAINT_NOT;
+
+        SvSetMagicSV(*padentry, val);


Go read the source code for macro SvSetMagicSV() and tell me if its optimizations apply or don't apply here. You need to set a breakpoint in your C debugger and examine left and right side SV ptrs to fix this line either through a code change or a comment saying you verified certain behavior can (if so when and % chance of it happening in typical production code), or that certain behavior will always happen here.

Go read the source code for macro SvSetMagicSV() and tell me if its optimizations apply or don't apply here. You need to set a breakpoint in your C debugger and examine left and right side SV ptrs to fix this line either through a code change or a comment saying you verified certain behavior can (if so when and % chance of it happening in typical production code), or that certain behavior will always happen here.

Also from the comment above, if right side SV is NULL, there is a better way to take an existing SV ptr and set it to undef. grep the repo's root .c and .h files to learn how. Since you want to fire SetMagic here (why and when can there be SMG here?), make sure the correct set to undef macro or fn either fires SMG for you or fire SMG yourself right after it returns.

bulk88 · 2025-08-15T16:38:20Z

pp.c

+
+        if(av_count(av)) {
+            /* see "target should be empty" comments in pp_argelem above */
+            av_refresh_elements_range(defav, parami, parami + argc);


should retval of av_count(av) be passed as arg 4 to av_refresh_elements_range()? does av_refresh_elements_range() need that info? dont look it up twice. IDK enough to say what to do here.

bulk88 · 2025-08-15T16:41:22Z

pp.c

+            if (UNLIKELY(TAINT_get) && !SvTAINTED(val))
+                TAINT_NOT;
+
+            av_store(av, avidx++, newSVsv(val));


newSVsv is not cow aware, see your code from above how to make it cow aware. dont use 64b ptrs on i386/arm32. newSVsv(&PL_sv_undef) is not how to alloc a new SV with value undef. use av_simple variants unless TIEARRAY can happen here.

bulk88 · 2025-08-15T16:44:06Z

pp.c

+            argc -= 2;
+
+            if (UNLIKELY(SvGMAGICAL(key)))
+                key = sv_mortalcopy(key);


#define sv_mortalcopy(sv) Perl_sv_mortalcopy_flags(aTHX_ sv, SV_GMAGIC|SV_DO_COW_SVSETSV) looks good on this line

bulk88 · 2025-08-15T16:48:26Z

pp.c

+            SV **svp;
+
+            svp = av_fetch(defav, parami, FALSE); parami++;
+            SV *key = svp ? *svp : &PL_sv_undef;


What on earth does hv_store_ent(hv, &PL_sv_undef, newSVsv(val), 0); mean?

how about executing hv_store_ent(hv, &PL_sv_undef, newSVsv(val), 0); 5 or 9 times in a row, each time with a different addr in var "val"? what does that mean? why is the previous sentence valid correct behavior and not a panic/heap corruption?

bulk88 · 2025-08-15T17:00:04Z

pp.c

+
+    if(!ok)
+        return cLOGOP->op_other;
+


this logic tree makes no sense. test each c auto or malloc backed variable exactly once in this if/else tree. var

OP* PL_op should be saved to a C auto at the very top of this PP func to stop multiple rereads.

PL_op is an lvalue but it is not a variable. If you believe its a C variable goto the top of pp.c and add this line. it will be harmless.

#include "EXTERN.h" #define PERL_IN_PP_C #include "perl.h" #include "keywords.h" #include "invlist_inline.h" #include "reentr.h" #include "regcharclass.h" #undef PL_op /* <<<<< Add this line and push this commit to blead, its a C variable right? */ /* variations on pp_null */ PP(pp_stub)

leonerd marked this pull request as draft August 14, 2025 20:21

mauke reviewed Aug 15, 2025

View reviewed changes

leonerd force-pushed the faster-signatures branch 3 times, most recently from 482ac37 to e961b24 Compare August 15, 2025 11:55

leonerd mentioned this pull request Aug 15, 2025

Assert sanity of name+len arguments to allocmy() #23583

Merged

leonerd added 5 commits August 15, 2025 14:59

Rename fields of struct yy_parser_signature for clarity

db01225

The previous names were just copied from similar fields in previous signature handling code. These new names are a better fit for their current and intended future purpose.

perly.y: Reduce repetition of signature scalar element grammar

fe37261

By defining a helper term for optional defaulting expression we can remove one of the three alternate cases down to just two. This will help when adding more code to these action blocks in future,

pp.c: Refactor some behaviours into helper functions

346b131

These two new helper functions will be useful for sharing behaviour with upcoming code, when adding OP_MULTIPARAM.

leonerd force-pushed the faster-signatures branch from e961b24 to 5dd6e3a Compare August 15, 2025 14:36

bulk88 reviewed Aug 15, 2025

View reviewed changes

lib/B/Deparse.pm

my $defop = $paramtest->first->first;

if ($defop->name eq "stub") {

$sig[$parami] .= "$assign";

Copy link

Contributor

bulk88 Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y the ""s?

bulk88 reviewed Aug 15, 2025

View reviewed changes

leonerd marked this pull request as ready for review August 15, 2025 16:57

bulk88 reviewed Aug 15, 2025

View reviewed changes

		char slurpy; /* the sigil of the slurpy var (or null) */
		OP elemops; / NULL, or an OP_LINESEQ of individual element and fence ops */

Create new OP_MULTIPARAM to implement subroutine signatures #23574

Are you sure you want to change the base?

Create new OP_MULTIPARAM to implement subroutine signatures #23574

Conversation

leonerd commented Aug 14, 2025

Uh oh!

leonerd commented Aug 14, 2025

Uh oh!

mauke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulk88 Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Create new `OP_MULTIPARAM` to implement subroutine signatures #23574

Create new `OP_MULTIPARAM` to implement subroutine signatures #23574

bulk88 Aug 15, 2025 •

edited

Loading